-
Notifications
You must be signed in to change notification settings - Fork 1
Feature: Use type safe view states #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
feature/home/src/main/kotlin/nl/q42/template/home/main/presentation/HomeViewState.kt
Show resolved
Hide resolved
Besides the inconvenient usage points discussed above, my main concern to open this PR is to discourage the type of mutable state that mixes concerns (a single ViewState class with many fields) from becoming a pattern in the codebase. My worry is that if a different developer takes the single class ViewState and organically starts adding state to the class, it will eventually lead to a complex mix of incongruent or invalid states. Let me illustrate my fears, suppose a dev adds to the original ViewState a couple more fields of data: data class HomeViewState(
val isLoading: Boolean = false,
val showError: Boolean = false,
val userEmailTitle: ViewStateString? = null,
val favoriteItems: List<ItemType>? = null,
val showLoginButton: Boolean = false
) When the ViewModel gets an authentication error, it should set The systemic problem here is that we are mixing, error + loading + content + authentication states into one single class and for every new field in this class we should create a matrix of combinations between all the fields to find all the illegal states and write defensive code to proactively prevent each on of those cases. The bugs that come from not proactively preventing each illegal state combination can be very tricky to reproduce and debug. Bug example from the existing code in main branch for fetchUser: fun fetchUser() {
viewModelScope.launch {
_uiState.update { it.copy(showError = false, isLoading = true) }
handleAction(
action = fetchUserUseCase(),
onError = { _uiState.update { it.copy(showError = true, isLoading = false) } },
onSuccess = { _uiState.update { it.copy(isLoading = false) } },
)
}
} If onSuccess = { _uiState.update { it.copy(isLoading = false) } }, |
Pfew, This is a hard one to have an opinion about. I tried this approach once and regretted it. But you make some good points. Let's discuss them during a android coffee or dev meet. I'd like to know what the other devs think. |
More input from an article explaining the gotchas of using both approaches and why sealed hierarchies are safer (including drawbacks and solutions) https://proandroiddev.com/how-to-safely-update-state-in-your-kotlin-apps-bf51ccebe2ef |
9851a7e
to
4ce0bb8
Compare
Great article! Okay, agreed! You've found a bug 👍 , and the article found many more, so let's go for the less developer friendly, but better approach! There are more possible bugs and improvements mentioned in the article, on state management, let's go fix these as well, in another PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great article!
Okay, agreed! You've found a bug 👍 , and the article found many more, so let's go for the less developer friendly, but better approach!
There are more possible bugs and improvements mentioned in the article, on state management, let's go fix these as well, in another PR?
import kotlinx.coroutines.flow.MutableStateFlow | ||
import kotlinx.coroutines.flow.update | ||
import nl.q42.template.data.user.local.model.UserEntity | ||
import javax.inject.Inject | ||
|
||
internal class UserLocalDataSource @Inject constructor() { | ||
|
||
private val userFlow = MutableStateFlow<UserEntity?>(null) // this is dummy code, replace it with your own local storage implementation. | ||
private val userFlow = MutableSharedFlow<UserEntity?>() // this is dummy code, replace it with your own local storage implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would the view need to render again when the viewstate did not change? I mean, nothing will change on the screen if the viewstate is equal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't reply on this yet, if there is no change, I don't even emit from data layer to presentation, usually. No change = the viewmodel usually does not have to know that we received an update with exactly the same data. We watch for changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's because the result data is not being set in the onSuccess
of the handleAction
https://github.com/Q42/Template.Android/pull/73/files#diff-2bda0f897c6e85b2e28560b67fed0d8de6c42573bccdc77325ddf559e0acc9efR60
but instead on the startObservingUserChanges
function
https://github.com/Q42/Template.Android/pull/73/files#diff-2bda0f897c6e85b2e28560b67fed0d8de6c42573bccdc77325ddf559e0acc9efR68
If we use a State flow, we will not receive duplicate updates, but then the UI state will stay in Loading mode because we don't update the UI in onSuccess
.
This solution is already merged (apologies I didn't see this message from last year), if you prefer to use a StateFlow, I think it would be better to get rid of the startObservingUserChanges
function and let the fetchUser
and handleAction
functions handle all the updates to the _uiState
.
feature/home/src/main/kotlin/nl/q42/template/home/main/presentation/HomeViewState.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebaslogen okay, I'd say let's fix the conflicts and merge it :-)
4ce0bb8
to
cd98fab
Compare
cd98fab
to
5c5069d
Compare
5c5069d
to
b4844c4
Compare
Why is this important?
To encourage the use of ViewStates that avoid illegal states (e.g. HomeViewState(userEmailTitle = "test@test.com", isLoading = true, showError = true) 🤯😕)
Notes
The MutableStateFlow in the repo needs to be a MutableSharedFlow, otherwise it won't emit new events when the same data is received from the backend, in this case we want to get updates and let the UI do the diffs.